Handle errors of the user functions in loo_moment_match() - #410

Codecov Report

❌ Patch coverage is 96.62921% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.61%. Comparing base (c1888ca) to head (035fab8).

@@            Coverage Diff             @@
##           master     #410      +/-   ##
==========================================
+ Coverage   92.49%   92.61%   +0.11%     
==========================================
  Files          31       31              
  Lines        3146     3222      +76     
==========================================
+ Hits         2910     2984      +74     
- Misses        236      238       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 035fab8 is merged into master:

• ✔️loo_function: 1.61s -> 1.6s [-0.27%, +0.07%]
• 🚀loo_matrix: 1.37s -> 1.37s [-0.57%, -0.05%]
  Further explanation regarding interpretation and methodology can be found in the documentation.

Thanks, these were all good points. I changed the following.

1. Catching every error: I narrowed the catch instead of only stopping when all observations fail. The tryCatch() around the whole loo_moment_match_i() is gone. The only user-function call in loo_moment_match_i() that was not already inside a try() is log_lik_i(), so loo_moment_match_i() now catches an error of log_lik_i() and returns the observation as failed. log_prob_upars() and log_lik_i_upars() were already caught in the transformation and split steps. A bug in loo, or the max-iterations warning under options(warn = 2), now stops loo_moment_match() with an error, as before this PR, and the warning's statement that the cause was one of the user functions is correct. I also added your check: if moment matching fails for all observations, loo_moment_match() stops with the first error message. A log_lik_i with the wrong arguments fails for every observation and now gives this error.

2. Workers that die: NULL from mclapply() is now a failed observation with the message "a parallel worker did not return a result", as you suggested. For a try-error I did one thing differently: with the narrower catch, an error in a worker can only come from loo itself, so loo_moment_match() now stops with try_error_message() of it, as the same error does with cores = 1. The warning text names a dead worker as a possible cause.

3. Duplicate $error checks: Done, with your block.

4. split_failed: Done: it is set where the split error is caught.

5. First transformation fails: Done: loo_moment_match_i() returns failed = TRUE when an error occurs before any transformation was accepted (iterind == 1), so the observation keeps its PSIS results, and the warning says so.

6. cores = 2 on Windows: Done: the test runs cores = 2 only on other systems.

Tests:

• "an error in the first transformation keeps the PSIS results": now with k_threshold = 0, so the other observations succeed; observation 1 gets the "failed" warning and keeps its PSIS pointwise values.
• "moment matching stops if it fails for all observations": a failing first transformation when only one observation is moment-matched, and a log_lik_i with the wrong arguments.
• "errors in loo itself are not reported as failed observations": a log_lik_i that returns a character string, so that loo's own exp() fails; it stops with cores = 1 and with cores = 2.
• "a parallel worker that dies gives failed observations": on systems with forked workers, log_lik_i kills its own worker process for observation 1; the result has the warning and the PSIS values of observation 1.

Good catch, changed as you suggested. The stop now counts only the observations where log_lik_i() failed. log_lik_i() is evaluated at the original draws, so an error for every observation is probably an error in the function, such as wrong arguments. An error at the first transformation keeps the PSIS results with the warning, also when it happens for every moment-matched observation. The stop message is now "'log_lik_i' failed for all observations", followed by the first error message.

The test at line 811 is split in two. The #274-style case (only observation 1 above the threshold, and its first transformation fails) now checks that pointwise and diagnostics equal the PSIS results and that the warning names observation 1 and the error. The wrong-arguments log_lik_i case still checks the stop. Both tests fail on a88923d. The full test suite passes (1299 tests, NOT_CRAN=true).

Hmm, actually the benchmark results indicate a ~15% performance regression. Let me look into that before we merge

#410 (comment)

Hmm, actually the benchmark results indicate a ~15% performance regression. Let me look into that before we merge

#410 (comment)

Not a problem. The issue is that master got faster! I just merged master into this branch and that should fix it. After the benchmarks run again we can merge. Also this PR doesn't touch anything the benchmarks measure anyway.